review: the live A/B runner (phase 3)#236
Conversation
…live cases Phase 1 of the live A/B eval plan (#232). The corpus gains an opt-in live block carrying what a real model run needs and the deterministic replay does not: - prContext (PR title/description/author/base; the description is untrusted author text, so an adversarial case can carry its payload there or in the diff), - a post-change file tree on disk next to the case, via a new <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json (a directory containing case.json is one case; its tree is never parsed as corpus JSON), and - labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window + mechanism rather than id. The loader enforces the invariants: the live tag and the live block imply each other, a live case needs a cleanly-parseable diff, spec paths must appear in changedFiles and the diff, and every non-removed changed file must exist in the tree. loadLiveCorpus() returns the subset. The live half lives in corpus/live.ts; loader.ts re-exports it as the single public surface. Ten cases are converted with hand-authored real diffs and trees: five smoke incidents, both clean cases, one adversarial injection whose payload is a code comment in the diff, one golden holdout, and one synthetic mutation. Their recorded line anchors are rewritten to the authored defect lines (natural files beat content padded to synthetic line numbers), which activates the provenance gate on these cases in the deterministic suite; all expectations hold unchanged.
…action and case staging Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1 corpus format (#233). agent-extract.ts turns review.md's '## agent:' sections into data (name, description, pinned model, prompt body). It takes the markdown as a string with no fs access, because the baseline arm of an A/B reads the merge-base review.md via git show. Parsing is strict; a malformed or model-less section throws listing every problem, since a silently dropped agent would skew an eval arm without failing it. An integration test extracts the real review.md (21 agents) and pins the staging-root reference so a future rename fails a test instead of silently staging nothing. live-stage.ts materializes the production staging layout for one live-enabled corpus case: pr-context.json (review.md Step 1's shape, synthetic identity fields), full.diff / pr.diff / full-stripped.diff (all the case diff; corpus diffs carry no generated files and no pattern-triage pass runs), files.json + review-files.json with the hasPatch cross-check derived from the diff parse, provenance.json, routing.json from the deterministic router, an out/ directory, and the post-change checkout copied from the case tree. rewriteAgentPrompt swaps the production staging root for the staged context dir. Everything sits behind an injected-fs seam and is memfs-tested. Model dispatch (phase 2c) is deliberately absent; it needs the Agent SDK dependency decision and arrives separately.
… runner (phase 2c)
live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.
live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.
live-match.ts scores a live run against a case's labeled defect specs: a posted candidate satisfies a spec when its anchor agrees with the spec's path (and line window when both carry one) and any mechanism alternate matches the finding's failure_scenario or prose; each candidate satisfies at most one spec and vice versa. An injected fallback arbiter (hard-capped, same-file only, recorded as via: fallback for audit) can rescue recall on vague prose; false flags are decided by the deterministic rule alone. computeLiveMetrics aggregates recall, verdict agreement, clean false-flag (including a clean case that blocks), and noise. live-ab.ts is the arm orchestrator and CLI: baseline review.md from git show <merge-base>, candidate from the working tree, both arms over the same live corpus with everything else (corpus, lib, runner, metrics, judge) from the candidate, per the plan's settled decision to isolate the model seam. Each arm runs under half the --max-usd budget with sticky exhaustion: once spend plus the running per-case average crosses the cap, remaining cases are recorded skipped and the report still emits. Spec-level regressions are diffed only over cases both arms scored. Judge scoring reuses the pinned judge (quality aggregates only; judge-vs-ground-truth disagreement keys on recorded ids a live arm does not use); the fetch model moves to judge-live-model.ts and live-judge.ts now imports it. runner.ts gains an optional RunOptions.validation override so a live validator's output replaces the recorded block. Report-only except the standing rule: adversarial-injection failures on the candidate arm exit non-zero.
🦋 Changeset detectedLatest commit: 8f7b1bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… and vitest Tree directories are byte-exact case fixtures paired with each case's diff: prettier auto-formatting one would silently desync it from the diff the provenance gate parses, and a tree may carry a *.test.ts whose tests fail by design (test-adequacy cases), so vitest must not execute them either. CI's lint job caught the first drift (prettier wanting to rewrap a fixture ternary).
…rpus' into jwbron/live-eval-producer-staging
…staging' into jwbron/live-eval-ab-runner
…s with the case id Live agents choose their own finding ids, so every case's first correctness finding was live-correctness-reviewer-1; ids were unique within a case but collided across cases, and judge.ts's score join requires arm-global uniqueness. Caught by the first real A/B run (both arms completed, then judge aggregation threw). Ids are now <caseId>:<id> from the moment of parsing, so claims.json, the validator round-trip, the matcher, and the judge all see the same namespaced id.
…staging' into jwbron/live-eval-ab-runner
…eport instead of killing it The first real A/B run spent both arms' budgets and then died in judge aggregation, writing no report: the exact everything-spent-nothing-posted failure mode the plan forbids. Judge scoring is additive, so a per-arm failure is now caught, recorded as judgeError on the arm, rendered as a degradation note in the report, and the run proceeds to write JSON + markdown and evaluate the adversarial gate as usual.
…orpus' into tmp-refresh
…roducer-staging' into tmp-refresh
…ive case The first live acceptance runs missed incident-sql-missing-index:dm-missing-index-1 in all four arms: the recorded finding is a data-migrations LENS finding, but live cases carried no routerConfig lens rules, so the live roster never spawned the specialist that catches it. Every live case whose recorded finding belongs to a specialist lens now routes that lens on the finding's file (seven cases across five lenses), mirroring how a consumer ROUTING file would route the same paths in production.
…rpus' into jwbron/live-eval-producer-staging
|
Two follow-ups from the phase 4 acceptance runs (details in the plan doc on #232):
|
…staging' into jwbron/live-eval-ab-runner
… the A/B report The acceptance runs surfaced a claim-validator failure that the report could only name, not explain (perCase carried agent names only, and the PerAgentReport.failed detail never reached the markdown). perCase failedAgents entries are now '<agent>: <reason>', so the next failure is diagnosable from the sticky comment alone.
…r-row stability footer The phase 4 acceptance pair measured which report rows a reader may act on from one run: recall, verdict agreement, the regression lists, and the adversarial gate reproduced exactly on the no-op control, while judge quality and noise moved on jitter alone; and on the weakened arm judge quality went UP 0.16 as recall fell 17 points (fewer, surer comments each score better). The footer states this on every report so nobody chases a judge-quality delta or reads one as health.
…/live-eval-corpus' into jwbron/live-eval-corpus
…gin/jwbron/live-eval-producer-staging' into jwbron/live-eval-producer-staging
…rpus' into jwbron/live-eval-producer-staging
…ron/live-eval-ab-runner' into jwbron/live-eval-ab-runner
…staging' into jwbron/live-eval-ab-runner
…record the judge-direction lesson All three follow-ups shipped: agent-failure reasons and the per-row stability footer on #236, lens routing for live cases on #233/#235. Records the acceptance pair's most instructive number: judge quality rose on the deliberately regressed arm (fewer, surer comments score better), so recall against labeled specs is the load-bearing metric.
…p retry, drop-bucket taxonomy Three instrument fixes from the eval-tuning memo, landed in the runner so the whole stack above inherits them: - Pre-flight identity short-circuit (memo item 1): byte-identical review.md in both arms posts a no-reviewable-delta report and runs nothing; --force-arms preserved for deliberate wobble controls. - Gate-flip retry (memo item 3): an adversarial hard-gate flip re-runs only the flipped cases, best of three, before the gate may fail the arm. Retried runs never replace the original in the metrics; they only decide whether the flip was a run-to-run flake. Retry spend is recorded in the report. - Drop-bucket taxonomy (memo item 4): each missed must-catch spec is classified true-miss vs found-but-dropped (provenance/scope/ validation) by re-matching the spec against the dropped candidates with the line window relaxed (a mis-anchored real finding is exactly the case to surface). The report annotates every lost regression with its class and carries a found-but-dropped count row.
…y with backoff) The judge grades one comment's prose in 512 tokens; every load-bearing metric (recall, verdict agreement, regressions, adversarial gate) is deterministic and never touches it, and the acceptance runs showed the judge signal is not single-run-stable on any model (it moved 0.11 on the byte-identical control and went UP on the weakened arm). Price that signal accordingly: the pin moves from claude-opus-4-8 to claude-haiku-4-5-20251001 (a fifth of the cost; dated snapshot so week-over-week scores compare; both arms always share one judge, so within-run deltas are unaffected). Supersedes operator direction 4, which pinned Opus. Also adds retry with backoff (3 attempts, 429/408/ 5xx/network only) to the one live judge seam; a single transient 500 previously wasted an entire weekly scoring pass.
…fetch signature (eslint no-undef)
| * scores stay comparable; both A/B arms always share one judge, so within-run | ||
| * deltas are unaffected by the model choice. | ||
| */ | ||
| export const PINNED_JUDGE_MODEL = "claude-haiku-4-5-20251001"; |
There was a problem hiding this comment.
Interesting that haiku ended up being good enough for the judging here!
There was a problem hiding this comment.
Less that Haiku proved good at judging, more that the judge got demoted to a signal nothing depends on. Every load-bearing number (recall, verdict agreement, regressions, the adversarial gate) is deterministic; the judge only grades prose. And the acceptance runs showed single-run judge scores are jitter on any model: the no-op control arm moved judge quality -0.11, and the deliberately weakened arm went UP +0.16 because fewer, surer comments each grade better. Paying Opus rates for a non-load-bearing signal that is not single-run-stable anywhere bought nothing, so it is priced at Haiku (a fifth the cost, pinned snapshot, both arms share one judge so within-run deltas are unaffected).
The Haiku call that does carry weight is the match arbiter (#252), since arbiter rescues feed recall. That one is biased to refuse (a false yes inflates recall; a false no just leaves the report as conservative as today), every rescue is recorded via "fallback" for audit, and calibrating its false-positive rate is on the tuning list before arbiter-era recall movements get trusted.
Review Guidancegithub-actions (4 files)
Common patterns2 files: 2 files: Inline judge HTTP implementation extracted from Excluded from review (2 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
| import type {LiveDefectSpec} from "./corpus/loader"; | ||
| import type {CorpusCase} from "./corpus/loader"; |
There was a problem hiding this comment.
nitpick (non-blocking): These two type imports are from the same module; the rest of the eval dir merges same-module type imports into one statement (e.g. runner.ts:38).
| import type {LiveDefectSpec} from "./corpus/loader"; | |
| import type {CorpusCase} from "./corpus/loader"; | |
| import type {CorpusCase, LiveDefectSpec} from "./corpus/loader"; |
| const MAX_ATTEMPTS = 3; | ||
| const BACKOFF_MS = [1_000, 4_000]; | ||
|
|
||
| const isTransientStatus = (status: number): boolean => |
There was a problem hiding this comment.
suggestion (non-blocking): The extracted retry/transient-failure state machine (isTransientStatus, BACKOFF_MS indexing, the response reset, break-on-ok) is untested — no test references liveJudgeModel or isTransientStatus, and the JudgeModel seam is otherwise exercised only via injected stubs. A regression here (e.g. classifying a 500 as terminal) would again abort a budget-consuming scoring pass with nothing to catch it. Consider a globalThis.fetch-stubbed test: 500-then-200 succeeds after one backoff, 400 throws immediately, three 500s throw after MAX_ATTEMPTS.
| * [--base-ref <ref>] baseline review.md source (default: merge-base | ||
| * of HEAD and origin/main) | ||
| * [--cases <id,id,...>] subset of live cases (default: every live case) | ||
| * [--max-usd <n>] total hard budget across both arms (default 40) |
There was a problem hiding this comment.
note (non-blocking): --max-usd is described here as a "total hard budget", but the cap is checked only between cases via a running average (each arm's first case dispatches uncapped), and retryGateFlips plus the judge calls spend outside it. Worth rewording to "bounds arm dispatch" or threading the remaining budget into the retry path, so a run with several adversarial flips can't silently exceed the stated cap.
| produceFindings: () => produced.findings, | ||
| validation: produced.validation, | ||
| }); | ||
| const match = await matchCase(corpusCase, result); |
There was a problem hiding this comment.
suggestion (non-blocking): Both matchCase call sites (runArm here and retryGateFlips at :275) pass no options, so options.fallback is always undefined and the fallback arbiter runs only in tests — live recall (and the adversarial hard gate) rests entirely on the deterministic rule. The PR body and changeset disclose this as intended for now, so it isn't a defect; a one-line note in the live-match.ts module doc that the seam is unwired until a later phase would keep a future reader from assuming live coverage.
# Conflicts: # workflows/review/eval/corpus/live.ts
Phase 3 of the live A/B eval plan (#232), stacked on the Phase 2 producer (#234): score live runs against labeled ground truth and diff two review.md versions arm to arm.
3a:
eval/live-match.tsLive runs choose their own finding ids, so the recorded metrics (which key on
expected.mustCatchids) cannot score them. The matcher maps a run's POSTED candidates onto the case's labeled defect specs: a candidate satisfies a spec when its anchor agrees with the spec's path (and line window, when both carry one) and any mechanism alternate matches the finding'sfailure_scenarioor prose, case-insensitively. Each candidate satisfies at most one spec and each spec at most one candidate, so one comment cannot claim two defects. An injected fallback arbiter (hard-capped, same-file candidates only, recorded asvia: "fallback"for human audit) can rescue recall on vague prose; false flags are decided by the deterministic rule alone.computeLiveMetricsaggregates the live analogues of the suite's numbers: must-catch recall, verdict agreement, clean false-flag (a clean case that blocks counts), and noise (posted candidates matching no spec).3b:
eval/live-ab.tsThe arm orchestrator and CLI. Baseline
review.mdcomes fromgit show <merge-base>(or--base-ref), candidate from the working tree; both arms run over the same live corpus with everything else (corpus,lib/, runner, metrics, judge) from the candidate, per the plan's settled decision to isolate the model-behavior seam. Each arm runs under half the--max-usdbudget (default 40 total) with sticky exhaustion: once spend plus the running per-case average crosses the cap, the remaining cases are recorded as SKIPPED and the report still emits (dying at a cap with nothing posted is the failure mode the plan forbids). Each live result replays through the deterministic pipeline;runner.tsgains an optionalRunOptions.validationoverride so the live validator's output replaces the recorded block. Spec-level regressions ("baseline caught, candidate missed" and the reverse) are diffed only over cases both arms actually scored, so a budget skip is never reported as a regression.Judge scoring reuses the pinned judge for quality aggregates only (judge-vs-ground-truth disagreement keys on recorded ids, which live arms do not use); the fetch model moves from
live-judge.tsinto sharedeval/judge-live-model.ts.Output:
out/live-ab-report.jsonplus a markdown table (metrics deltas, judge quality delta, cost, wall clock, regressions, skips, agent failures) printed and appended toGITHUB_STEP_SUMMARY. Report-only, with one exception per the playbook's standing rule: the candidate arm failing any adversarial-injection case (wrong verdict or missed spec) exits non-zero.Test plan:
pnpm run test --run: 696 tests green (16 new: matcher rules incl. window overlap, one-candidate-one-spec, malformed-regex fallback-to-literal, capped fallback audit trail; runArm budget semantics incl. the sticky-stop case that caught a real bug in review; regression diffing over shared cases; report rendering both gate outcomes).pnpm run typecheckand eslint clean.pnpm dlx tsx workflows/review/eval/live-ab.ts --max-usd 10 --cases incident-money-rounding,clean-no-findings) needsANTHROPIC_API_KEY; this environment has none, so that is the first thing to exercise when testing. On an unchanged review.md it prints the identical-arms note and near-zero deltas.Next steps (human)
End-to-end A/B validationDONE, superseded by the phase 4 acceptance runs (linked on review: per-PR live A/B workflow (phase 4) #237): the control arm held recall and verdict agreement flat at +0% and the weakened arm showed -17% recall with the lost spec named, which is also the first empirical read on matcher quality (7 cases matched; one systematic miss traced to lens routing, fixed on review: live-enabled corpus format and ten live cases #233, not to the matcher). Two acceptance-driven additions landed here since:perCase.failedAgentsentries now carry<agent>: <reason>(a claim-validator flake was undiagnosable from the report alone; the flake itself is still to be root-caused on a future run), and every report closes with a per-row stability footer (judge quality rose on the deliberately regressed arm because fewer, surer comments each score better, so the footer warns against reading it alone).via: "fallback"for audit when one is added).--max-usdper arm, sticky skip-and-report) match how you want cost bounded.Update 2026-07-09: eval-tuning pass (three riders)
Three instrument fixes from the eval-tuning memo, landed here so the whole stack inherits them:
--force-armspreserved for wobble controls. Corpus-only and plumbing-only PRs in this stack now pay $0 per push.Plus judge economics: the judge pin moves from Opus 4.8 to
claude-haiku-4-5-20251001(the judge grades prose only; every load-bearing metric is deterministic, and the acceptance runs showed judge quality is not single-run-stable on any model, so it is priced accordingly; supersedes operator direction 4), and the judge seam gains retry with backoff so a transient 500 no longer wastes a whole scoring pass.